Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix derive(PartialOrd) and optimise final field operation #49881

Merged
merged 9 commits into from
Apr 15, 2018

Conversation

varkor
Copy link
Member

@varkor varkor commented Apr 11, 2018

// Before (`lt` on 2-field struct)
self.f1 < other.f1 || (!(other.f1 < self.f1) &&
(self.f2 < other.f2 || (!(other.f2 < self.f2) &&
(false)
))
)

// After
self.f1 < other.f1 || (!(other.f1 < self.f1) &&
self.f2 < other.f2
)

// Before (`le` on 2-field struct)
self.f1 < other.f1 || (!(other.f1 < self.f1) &&
(self.f2 < other.f2 || (!(other.f2 < self.f2) &&
(true)
))
)

// After
self.f1 < other.f1 || (self.f1 == other.f1 &&
self.f2 <= other.f2
)

(The big diff is mainly because of a past faulty rustfmt application that I corrected 😒)

Fixes #49650 and fixes #49505.

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 11, 2018
@rust-lang rust-lang deleted a comment from TimNN Apr 11, 2018
@cramertj
Copy link
Member

@aturon is on vacation, so r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned aturon Apr 11, 2018
@varkor
Copy link
Member Author

varkor commented Apr 11, 2018

I might also look into reducing the number of comparison operations further, following this suggestion. But I'm not sure if I'll get to it right away.

@rust-lang rust-lang deleted a comment from TimNN Apr 11, 2018
@rust-lang rust-lang deleted a comment from TimNN Apr 11, 2018
These now spit out errors for `<=` and `>=` as well.
@rust-lang rust-lang deleted a comment from TimNN Apr 11, 2018
@eddyb
Copy link
Member

eddyb commented Apr 11, 2018

r? @alexcrichton or @Manishearth

@rust-highfive rust-highfive assigned alexcrichton and unassigned eddyb Apr 11, 2018
let not_cmp = cx.expr_unary(span,
ast::UnOp::Not,
cx.expr_binary(span, op, other_f.clone(), self_f));
let deleg_cmp = if !equal {
Copy link
Member

@Manishearth Manishearth Apr 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the comment above with the new generated code for the <= case

}
}

/// Special version of `cs_fold` that uses the result of a function call on the first field
/// as the base case when is at least 1 field, and the usual base case when there are zero fields.
pub fn cs_fold1<F, B>(use_foldl: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs clearer documentation

let eq = cx.expr_binary(span, BinOpKind::Ne, self_f, other_f.clone());
cx.expr_binary(span, BinOpKind::Or, subexpr, eq)
},
|cx, args| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this and the previous closure can be factored out into a function

@Manishearth
Copy link
Member

Overall looks good, minor nits

(1, Some(o_f)) => o_f,
_ => cx.span_bug(span, "not exactly 2 arguments in `derive(PartialEq)`"),
};
let eq = |cx: &mut ExtCtxt, span: Span, self_f: P<Expr>, other_fs: &[P<Expr>]| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant splitting this out into a function such that it can be shared between the Eq and Neq code

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh... that's another good idea! 😅

@alexcrichton
Copy link
Member

r? @Manishearth

@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 12, 2018

📌 Commit 105c518 has been approved by Manishearth

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 12, 2018
@bors
Copy link
Contributor

bors commented Apr 15, 2018

⌛ Testing commit 105c518 with merge bc001fa...

bors added a commit that referenced this pull request Apr 15, 2018
Fix derive(PartialOrd) and optimise final field operation

```rust
// Before (`lt` on 2-field struct)
self.f1 < other.f1 || (!(other.f1 < self.f1) &&
(self.f2 < other.f2 || (!(other.f2 < self.f2) &&
(false)
))
)

// After
self.f1 < other.f1 || (!(other.f1 < self.f1) &&
self.f2 < other.f2
)

// Before (`le` on 2-field struct)
self.f1 < other.f1 || (!(other.f1 < self.f1) &&
(self.f2 < other.f2 || (!(other.f2 < self.f2) &&
(true)
))
)

// After
self.f1 < other.f1 || (self.f1 == other.f1 &&
self.f2 <= other.f2
)
```

(The big diff is mainly because of a past faulty rustfmt application that I corrected 😒)

Fixes #49650 and fixes #49505.
@bors
Copy link
Contributor

bors commented Apr 15, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: Manishearth
Pushing bc001fa to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
8 participants